ext/pgsql: route pg_copy_from table_name through build_tablename by iliaal · Pull Request #21985 · php/php-src

iliaal · 2026-05-08T10:53:20Z

Add the table validation added in bug #62978 (which fixed pg_insert, pg_update, pg_select, pg_delete) but missed pg_copy_from. Not updating pg_copy_to since it allows (query) for the table name per ext/pgsql/tests/06_bug73498.phpt.

devnexen · 2026-05-09T03:53:17Z

Thanks for starting the work, but as hinted. it is preferable you really go through. I ll put some comments.

devnexen · 2026-05-09T03:58:44Z

I want to see $null_as parameter being challenged too, e.g.

pg_query($db, "DROP TABLE IF EXISTS table1");
  pg_query($db, "DROP TABLE IF EXISTS injected");
  pg_query($db, "CREATE TABLE table1 (v text)");

  $null_as = "X'; CREATE TABLE injected (v text); --";

  pg_copy_from($db, 'victim', ["row\n"], "\t", $null_as);
  
  $r = pg_query($db, "SELECT 1 FROM pg_tables WHERE tablename='injected'");
  var_dump(pg_num_rows($r));  => int(1)

devnexen · 2026-05-09T04:00:03Z

Add the table validation added in bug #62978 (which fixed pg_insert, pg_update, pg_select, pg_delete) but missed pg_copy_from. Not updating pg_copy_to since it allows (query) for the table name per ext/pgsql/tests/06_bug73498.phpt.

pg_copy_to needs to go through the grinder too.

The COPY query embedded the table_name argument with a raw "%s" and the delimiter and null marker inside literal E'..' wrappers, so caller-supplied strings could break out and run side queries. Route bare table names through build_tablename (the same helper pg_insert/update/select/delete have used since bug #62978), and pass the delimiter and null marker through PQescapeLiteral. pg_copy_to keeps the parenthesised (query) source form documented in bug 73498, but wraps it in an extra paren pair so a string like (SELECT 1); DROP TABLE x; -- becomes a syntax error inside the outer parens instead of escaping out into a second statement. Closes phpGH-21985

iliaal · 2026-05-09T13:13:06Z

Both null_as and the delimiter now go through PQescapeLiteral, dropping the E'...' wrapper. Bare table_name in pg_copy_to routes through build_tablename. The (query) form per bug 73498 still passes through, but wrapped in an extra paren pair so something like (SELECT 1); DROP TABLE x; becomes a syntax error inside the outer parens instead of escaping out into a second statement.

devnexen · 2026-05-09T15:18:43Z

 	char *pg_null_as = "\\\\N";
-	size_t pg_null_as_len = 0;
-	char *query;
+	size_t pg_null_as_len = sizeof("\\\\N") - 1;


hmmmm ... can you try a test that looks like this ?

--TEST-- pg_copy_to() / pg_copy_from() default null marker is "\N" --EXTENSIONS-- pgsql --SKIPIF-- <?php include("inc/skipif.inc"); ?> --FILE-- <?php include('inc/config.inc'); $t = "pg_copy_default_null"; $db = pg_connect($conn_str); pg_query($db, "DROP TABLE IF EXISTS {$t}"); pg_query($db, "CREATE TABLE {$t} (id int, v text)"); pg_query($db, "INSERT INTO {$t} VALUES (1, 'hello'), (2, NULL)"); $rows = pg_copy_to($db, $t); var_dump($rows); pg_query($db, "DELETE FROM {$t}"); var_dump(pg_copy_from($db, $t, $rows)); var_dump(pg_fetch_all(pg_query($db, "SELECT v FROM {$t} ORDER BY id"))); ?> --CLEAN-- <?php include('inc/config.inc'); $db = pg_connect($conn_str); pg_query($db, "DROP TABLE IF EXISTS pg_copy_default_null"); ?> --EXPECT-- array(2) { [0]=> string(8) "1 hello " [1]=> string(4) "2 \N " } bool(true) array(2) { [0]=> array(1) { ["v"]=> string(5) "hello" } [1]=> array(1) { ["v"]=> NULL } }

The COPY query embedded the table_name argument with a raw "%s" and the delimiter and null marker inside literal E'..' wrappers, so caller-supplied strings could break out and run side queries. Route bare table names through build_tablename (the same helper pg_insert/update/select/delete have used since bug #62978), and pass the delimiter and null marker through PQescapeLiteral. pg_copy_to keeps the parenthesised (query) source form documented in bug 73498, but wraps it in an extra paren pair so a string like (SELECT 1); DROP TABLE x; -- becomes a syntax error inside the outer parens instead of escaping out into a second statement. Closes phpGH-21985

iliaal · 2026-05-09T15:26:55Z

Ah yes, good catch. Just \\N is enough here.

devnexen · 2026-05-09T15:44:30Z

other things to think about (no rush, won t be committed today), what happens when query has already parens, your test need stricter output expectations ; i.e. pg_copy_from(): %s ... your new error messages need to be more specific, aka which parameter had failed.

devnexen · 2026-05-09T15:56:36Z

+	if (!escaped_delimiter || !escaped_null_as) {
+		php_error_docref(NULL, E_WARNING, "Failed to escape COPY parameters");
+		if (escaped_delimiter) PQfreemem(escaped_delimiter);
+		if (escaped_null_as) PQfreemem(escaped_null_as);


nit: code style in php-src is new line and braces even with just 1 line block

Bah 😆 ok something to tweak

iliaal · 2026-05-09T16:04:12Z

other things to think about (no rush, won t be committed today), what happens when query has already parens, your test need stricter output expectations ; i.e. pg_copy_from(): %s ... your new error messages need to be more specific, aka which parameter had failed.

If already has parens shouldn't be an issue would just be double parens, I tested locally but let me double check and add to test.

The COPY query embedded the table_name argument with a raw "%s" and the delimiter and null marker inside literal E'..' wrappers, so caller-supplied strings could break out and run side queries. Route bare table names through build_tablename (the same helper pg_insert/update/select/delete have used since bug #62978), and pass the delimiter and null marker through PQescapeLiteral. pg_copy_to keeps the parenthesised (query) source form documented in bug 73498, but wraps it in an extra paren pair so a string like (SELECT 1); DROP TABLE x; -- becomes a syntax error inside the outer parens instead of escaping out into a second statement. Closes phpGH-21985

devnexen · 2026-05-09T16:45:14Z

+
+	char *escaped_delimiter = PQescapeLiteral(pgsql, ZSTR_VAL(pg_delimiter), 1);
+	if (!escaped_delimiter) {
+		php_error_docref(NULL, E_WARNING, "Failed to escape delimiter");


as mentioned, you have the specific information available, e.g. here pg_delimiter.

Maybe I didn't quite get the ask here, you want the error message to include the delimiter itself, seems of limited value to me, but can be added I suppose

yes just for clarification's sake.

or even better...
zend_string *msgbuf = _php_pgsql_trim_message(PQerrorMessage(pgsql));
php_error_docref...
zend_string_release(msgbuf);

Applied PQerrorMessage across all four errors and added params where needed, also for consistency promoted table escape error from notice to warning

devnexen · 2026-05-09T17:08:30Z

You covered most of the points I ll have a fresh look in the following days. Cheers !

The COPY query embedded the table_name argument with a raw "%s" and the delimiter and null marker inside literal E'..' wrappers, so caller-supplied strings could break out and run side queries. Route bare table names through build_tablename (the same helper pg_insert/update/select/delete have used since bug #62978), and pass the delimiter and null marker through PQescapeLiteral. pg_copy_to keeps the parenthesised (query) source form documented in bug 73498, but wraps it in an extra paren pair so a string like (SELECT 1); DROP TABLE x; -- becomes a syntax error inside the outer parens instead of escaping out into a second statement. Closes phpGH-21985

devnexen · 2026-05-11T03:30:18Z

+		smart_str_appendc(&querystr, '(');
+		smart_str_append(&querystr, table_name);
+		smart_str_appendc(&querystr, ')');
+	} else if (build_tablename(&querystr, pgsql, table_name) == FAILURE) {


(re)using build_tablename narrows what these accept. old code passed table_name through raw via spprintf so callers could tack on any COPY syntax. now anything that isn't a plain identifier or schema.table gets wrapped as one quoted name and fails. Before:

"mytable (col1, col2)" column list

"ONLY mytable"

'"my.table"' quoted name with a dot, memchr splits it

the (query) branch covers column lists but not ONLY or dotted quoted names. either gate build_tablename behind a simple-identifier check and error out, or add
explicit columns/only params.

Added a strict gate ahead of build_tablename: bare table_name must match [A-Za-z_][A-Za-z0-9_]* or schema.table, anything else (column list, ONLY, quoted, quoted-with-literal-dot) returns false with a warning pointing at the argument. Column lists and ONLY for pg_copy_to remain reachable via the (query) form.

devnexen · 2026-05-11T03:30:38Z

 	}
+	smart_str querystr = {0};
+	smart_str_appends(&querystr, "COPY ");
+	if (build_tablename(&querystr, pgsql, table_name) == FAILURE) {


Same gate applied here.

devnexen · 2026-05-11T03:31:22Z

+
+$rows = pg_fetch_all(pg_query($db, 'SELECT v FROM pg_copy_from_other')) ?: [];
+var_dump($rows);
+


can you add a case that tries to close the wrapper early, like '(SELECT 1)); DROP TABLE pg_copy_to_qsource; --', plus the pg_tables check?

Added the wrapper-close test case plus a paren-balance check on the (query) input: must start with (, end with ), and depth must reach 0 only at the final character. The original (SELECT 1); DROP...; -- case also hits the new gate, so its expected output flipped from a Postgres syntax error to the validation warning.

The COPY query embedded table_name with a raw "%s" and the delimiter and null marker inside literal E'..' wrappers, so caller-supplied strings could break out and run side queries. Route bare table names through build_tablename (the same helper pg_insert/update/select/delete have used since bug #62978) and pass the delimiter and null marker through PQescapeLiteral. pg_copy_to keeps the parenthesised (query) source form documented in bug 73498; the input is now SQL-aware paren-balance checked (single, double, and dollar-quoted literals are tracked) and rejected if depth returns to 0 before end-of-string, so the user-supplied string cannot close the wrapper early and inject a trailing statement. Closes phpGH-21985

The COPY query embedded table_name with a raw "%s" and the delimiter and null marker inside literal E'..' wrappers, so caller-supplied strings could break out and run side queries. Restrict the bare table_name argument to a plain identifier or schema.table (each side matching [A-Za-z_][A-Za-z0-9_]*) and route it through build_tablename, the same helper pg_insert/update/select/delete have used since bug #62978. Reject anything else with a clear warning so previously silent-fail cases (column lists, ONLY, quoted identifiers) point at the table_name argument instead of surfacing as a Postgres relation lookup error. Pass the delimiter and null marker through PQescapeLiteral. pg_copy_to keeps the parenthesised (query) source form documented in bug 73498; the input is now SQL-aware paren-balance checked (single, double, and dollar-quoted literals are tracked) and rejected if depth returns to 0 before end-of-string, so the user-supplied string cannot close the wrapper early and inject a trailing statement. Closes phpGH-21985

The COPY query embedded table_name with a raw "%s" and the delimiter and null marker inside literal E'..' wrappers, so caller-supplied strings could break out and run side queries. Restrict the bare table_name argument to a plain identifier or schema.table (each side matching [A-Za-z_][A-Za-z0-9_]*) and route it through build_tablename, the same helper pg_insert/update/select/delete have used since bug #62978. Reject anything else with a clear warning so previously silent-fail cases (column lists, ONLY, quoted identifiers) point at the table_name argument instead of surfacing as a Postgres relation lookup error. Pass the delimiter and null marker through PQescapeLiteral. pg_copy_to keeps the parenthesised (query) source form documented in bug 73498; the input is required to start with "(", end with ")", and the matching close paren must be the final character, so the user-supplied string cannot close the wrapper early and inject a trailing statement. Closes phpGH-21985

iliaal requested a review from devnexen as a code owner May 8, 2026 10:53

iliaal mentioned this pull request May 8, 2026

ext/pgsql: route pg_copy_from table_name through build_tablename iliaal/php-src#55

Closed

github-actions Bot added the Extension: pgsql label May 8, 2026

iliaal mentioned this pull request May 9, 2026

ext/pgsql: escape table name, delimiter, null marker in pg_copy_from/to iliaal/php-src#56

Closed

iliaal force-pushed the fix/pgsql-copy-from-table-name-escape branch from 1f0a1fa to bc6f630 Compare May 9, 2026 13:10

devnexen reviewed May 9, 2026

View reviewed changes

iliaal force-pushed the fix/pgsql-copy-from-table-name-escape branch from bc6f630 to 6fbe705 Compare May 9, 2026 15:26

devnexen reviewed May 9, 2026

View reviewed changes

iliaal force-pushed the fix/pgsql-copy-from-table-name-escape branch from 6fbe705 to 7ff2c22 Compare May 9, 2026 16:21

devnexen reviewed May 9, 2026

View reviewed changes

iliaal force-pushed the fix/pgsql-copy-from-table-name-escape branch from 7ff2c22 to a6dfbfb Compare May 9, 2026 17:13

iliaal force-pushed the fix/pgsql-copy-from-table-name-escape branch from a6dfbfb to 29fce0c Compare May 9, 2026 17:25

devnexen reviewed May 11, 2026

View reviewed changes

iliaal force-pushed the fix/pgsql-copy-from-table-name-escape branch from 29fce0c to b97ce88 Compare May 11, 2026 11:54

iliaal force-pushed the fix/pgsql-copy-from-table-name-escape branch from b97ce88 to 5bf0309 Compare May 11, 2026 12:04

iliaal force-pushed the fix/pgsql-copy-from-table-name-escape branch from 5bf0309 to 9b2015b Compare May 11, 2026 12:23


		$rows = pg_fetch_all(pg_query($db, 'SELECT v FROM pg_copy_from_other')) ?: [];
		var_dump($rows);

Conversation

iliaal commented May 8, 2026

Uh oh!

devnexen commented May 9, 2026

Uh oh!

devnexen commented May 9, 2026

Uh oh!

devnexen commented May 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

iliaal commented May 9, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

iliaal commented May 9, 2026

Uh oh!

devnexen commented May 9, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

iliaal commented May 9, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

iliaal May 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

devnexen commented May 9, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

devnexen commented May 9, 2026 •

edited

Loading

iliaal May 9, 2026 •

edited

Loading